fix($httpBackend): compare timeoutId with undefined in completeRequest #9979
Conversation
|
I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS. Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match. If you signed the CLA as a corporation, please let us know the company's name. Thanks a bunch! PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR. |
|
@gyx1000 could you please share a test with a scenario under which this issue shows up? This would tremendously help with reviewing suggester code change. BTW, before it get merged we would need a test so we are not introducing regressions in the future. Thnx! |
|
Oh, I see, this is due to https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L65 OK, I understand the pb and the solution sounds good - still we will need a test before it can land. |
|
@pkozlowski-opensource thanks for review. I discovered this problem when I tried to fix the todo https://github.com/angular/angular.js/blob/master/test/ng/httpBackendSpec.js#L10 proposed by @vojtajina. If the $browser.defer mock is used instead of "fakeTimeout", the test https://github.com/angular/angular.js/blob/master/test/ng/httpBackendSpec.js#L221 use the fix. Complete todo in this PR is acceptable for the needed test ? |
| @@ -126,7 +126,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc | |||
|
|
|||
| function completeRequest(callback, status, response, headersString, statusText) { | |||
| // cancel timeout and subsequent timeout promise resolution | |||
| timeoutId && $browserDefer.cancel(timeoutId); | |||
| timeoutId !== undefined && $browserDefer.cancel(timeoutId); | |||
There was a problem hiding this comment.
void 0, rather than undefined please
There was a problem hiding this comment.
also, please write this as if (timeoutId !== void 0) $browserDefer.cancel(timeoutId); --- it's much cleaner this way. (note that we need braces around the block IIRC)
|
I've submitted the fixup as a PR against @gyx1000's branch too, so it should be easy to do this one way or the other. fixup look good to you @pkozlowski-opensource? |
gyx1000
force-pushed
the
compare-timeoutid-with-undefined-in-completerequest
branch
from
986c69a to
db8ec99
Compare
gyx1000
force-pushed
the
compare-timeoutid-with-undefined-in-completerequest
branch
2 times, most recently
from
ebc5f32 to
c93af0f
Compare
httpBackend with ngMock browser.defer can never cancel the first deferredFn because the timeoutId returned by defer for the first fn is a zero value. Compare timeoutId with undefined fix this issue.
gyx1000
force-pushed
the
compare-timeoutid-with-undefined-in-completerequest
branch
from
c93af0f to
00182c8
Compare
|
OK, since the fixup has a test let's move the discussion in there (#9993). |
httpBackend with ngMock browser.defer can never cancel the first deferredFn because the timeoutId returned by defer for the first fn is a zero value.
Compare timeoutId with undefined fix this issue.